Skip to content

Conversation

@DeTuksa
Copy link

@DeTuksa DeTuksa commented Nov 17, 2025

Resolves: #5763

  • Updated packages/router-core/src/router.ts to treat 'notFound' like 'error' in invalidate
  • Added a new test in packages/react-router/tests/router.test.tsx that verifies a route returning a 404 notFound

Summary by CodeRabbit

  • New Features

    • Added a public invalidate capability to force routes to reset and re-run their loaders when triggered.
  • Bug Fixes

    • Routes previously stuck in a "not found" or error state now properly reset and re-run their loaders when invalidated, ensuring correct content or not-found UI is shown.
  • Tests

    • Added regression tests verifying not-found routes re-run loaders and continue to display the appropriate not-found content after invalidation.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 17, 2025

Walkthrough

Adds a public RouterCore.invalidate(...) method and updates commit/invalidate logic to treat matches with status 'notFound' like 'error' for invalidation (clearing errors and resetting to 'pending'). Adds tests ensuring loaders that throw or return notFound() re-run on filtered or global invalidation and notFoundComponent continues rendering.

Changes

Cohort / File(s) Summary
Router core
packages/router-core/src/router.ts
Added public invalidate(opts?) on RouterCore; changed commit logic so exiting matches with status === 'error' or 'notFound' are excluded from cachedMatches; invalidation now clears errors and converts 'notFound'/'error' matches to 'pending' (optionally forcePending) so loaders re-run.
Tests (react)
packages/react-router/tests/router.test.tsx
Added two regression tests in the "invalidate" suite covering HMR-filtered and global invalidation where loaders throw/return notFound(), asserting loaders re-run and notFoundComponent remains rendered.
Tests (solid)
packages/solid-router/tests/router.test.tsx
Added parallel regression tests mirroring the React tests: HMR-filtered and global invalidation re-running loaders that throw/return notFound() and verifying notFoundComponent rendering and loader invocation counts.

Sequence Diagram(s)

sequenceDiagram
    actor Dev
    participant HMR as HMR System
    participant Router
    participant Loader
    participant UI

    Dev->>HMR: Save file (HMR)
    HMR->>Router: call invalidate(filter?)
    activate Router
    Router->>Router: find matches matching filter
    alt match is 'error' or 'notFound' or forcePending
        Router->>Router: clear error, set status -> 'pending'
    end
    Router->>Loader: invoke loader for pending match
    activate Loader
    Loader-->>Router: returns data or throws/returns notFound()
    deactivate Loader
    Router->>UI: render route or notFoundComponent accordingly
    deactivate Router
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Pay special attention to packages/router-core/src/router.ts (invalidate API surface, status transitions, and cachedMatches commit changes).
  • Review the new tests in both React and Solid packages for correct coverage and flakiness risk (HMR filter simulation, loader invocation counts).
  • Check exported type/signature consistency for the new invalidate method.

Possibly related PRs

Suggested reviewers

  • nlynzaad
  • schiller-manuel

Poem

🐰 A hop, a patch, a tidy nudge,
NotFound held fast, the loaders trudge.
HMR knocks, the router sighs,
Re-runs the loader, keeps the prize. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main issue being fixed: routes with server functions that throw notFound() crashing during HMR, which matches the substantial code changes and test additions across multiple packages.
Linked Issues check ✅ Passed The PR successfully addresses issue #5763 by treating notFound like error in invalidation logic, preventing crashes on HMR while preserving notFoundComponent rendering in both react-router and solid-router.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the notFound HMR crash: router-core invalidate logic, test coverage for both react-router and solid-router validate the fix without unrelated modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a8f1364 and c4f892b.

📒 Files selected for processing (2)
  • packages/router-core/src/router.ts (3 hunks)
  • packages/solid-router/tests/router.test.tsx (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.

Applied to files:

  • packages/solid-router/tests/router.test.tsx
  • packages/router-core/src/router.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.

Applied to files:

  • packages/router-core/src/router.ts
🔇 Additional comments (3)
packages/solid-router/tests/router.test.tsx (1)

1021-1101: LGTM! Comprehensive tests for notFound invalidation behavior.

Both test cases thoroughly verify that loaders throwing or returning notFound() are properly re-run after invalidation, and that the notFoundComponent continues to render correctly. The tests cover:

  • HMR-style filtered invalidation (lines 1021-1062)
  • Global invalidation without filter (lines 1064-1101)
  • Both throw notFound() and return notFound() patterns
  • Proper loader re-execution tracking via mock call counts
  • UI state verification (notFoundComponent present, route component absent)
packages/router-core/src/router.ts (2)

2133-2144: LGTM! Cache filtering correctly excludes error and notFound matches.

The updated logic properly prevents matches with status: 'error' or status: 'notFound' from being cached, ensuring that subsequent invalidations or reloads will re-run their loaders instead of reusing failed/not-found data. This is consistent with the invalidate behavior that resets these statuses to 'pending'.

Note: This change was already reviewed and addressed in previous commits (2794cf0 to a8f1364).


2316-2357: LGTM! Invalidate method correctly treats notFound like error status.

The updated invalidate method properly resets matches with status: 'notFound' to 'pending' (clearing their error) alongside matches with status: 'error'. This ensures that loaders which threw or returned notFound() are re-run on the next load() call, fixing the HMR crash described in issue #5763.

The comprehensive JSDoc clearly documents the behavior, and the implementation is consistent with the cache filtering logic at lines 2141-2144.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud
Copy link

nx-cloud bot commented Nov 18, 2025

🤖 Nx Cloud AI Fix Eligible

An automatically generated fix could have helped fix failing tasks for this run, but Self-healing CI is disabled for this workspace. Visit workspace settings to enable it and get automatic fixes in future runs.

To disable these notifications, a workspace admin can disable them in workspace settings.


View your CI Pipeline Execution ↗ for commit c4f892b

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ❌ Failed 9m 6s View ↗

☁️ Nx Cloud last updated this comment at 2025-11-19 11:14:31 UTC

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3cd2f15 and 68e25a9.

📒 Files selected for processing (2)
  • packages/react-router/tests/router.test.tsx (1 hunks)
  • packages/router-core/src/router.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/react-router/tests/router.test.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.

Applied to files:

  • packages/router-core/src/router.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.

Applied to files:

  • packages/router-core/src/router.ts

@birkskyum
Copy link
Member

@DeTuksa , can you add a similar test in the solid-router?

@DeTuksa
Copy link
Author

DeTuksa commented Nov 18, 2025

Sure @birkskyum😀

I added these cases:

it('re-runs loaders that throw notFound() when invalidated via HMR filter', async () => {
    const history = createMemoryHistory({
      initialEntries: ['/hmr-not-found'],
    })
    const loader = vi.fn(() => {
      throw notFound()
    })

    const rootRoute = createRootRoute({
      component: () => <Outlet />,
    })

    const hmrRoute = createRoute({
      getParentRoute: () => rootRoute,
      path: '/hmr-not-found',
      loader,
      component: () => <div data-testid="hmr-route">Route</div>,
      notFoundComponent: () => (
        <div data-testid="hmr-route-not-found">Route Not Found</div>
      ),
    })

    const router = createRouter({
      routeTree: rootRoute.addChildren([hmrRoute]),
      history,
    })

    render(() => <RouterProvider router={router} />)
    await router.load()

    await screen.findByTestId('hmr-route-not-found')
    const initialCalls = loader.mock.calls.length
    expect(initialCalls).toBeGreaterThan(0)

    await router.invalidate({
      filter: (match) => match.routeId === hmrRoute.id,
    })

    await waitFor(() => expect(loader).toHaveBeenCalledTimes(initialCalls + 1))
    await screen.findByTestId('hmr-route-not-found')
    expect(screen.queryByTestId('hmr-route')).not.toBeInTheDocument()
  })

  it('keeps rendering a route notFoundComponent when loader returns notFound() after invalidate', async () => {
    const history = createMemoryHistory({
      initialEntries: ['/loader-not-found'],
    })
    const loader = vi.fn(() => notFound())

    const rootRoute = createRootRoute({
      component: () => <Outlet />,
    })

    const loaderRoute = createRoute({
      getParentRoute: () => rootRoute,
      path: '/loader-not-found',
      loader,
      component: () => <div data-testid="loader-route">Route</div>,
      notFoundComponent: () => (
        <div data-testid="loader-not-found-component">Route Not Found</div>
      ),
    })

    const router = createRouter({
      routeTree: rootRoute.addChildren([loaderRoute]),
      history,
    })

    render(() => <RouterProvider router={router} />)
    await router.load()

    await screen.findByTestId('loader-not-found-component')
    const initialCalls = loader.mock.calls.length
    expect(initialCalls).toBeGreaterThan(0)

    await router.invalidate()

    await waitFor(() => expect(loader).toHaveBeenCalledTimes(initialCalls + 1))
    await screen.findByTestId('loader-not-found-component')
    expect(screen.queryByTestId('loader-route')).not.toBeInTheDocument()
  })

@DeTuksa
Copy link
Author

DeTuksa commented Nov 19, 2025

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 19, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@DeTuksa
Copy link
Author

DeTuksa commented Nov 19, 2025

@birkskyum I think the checks that failed are related to the cloudfare downtime yesterday😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Route with server function as loader that throws notFound crashes route on HMR

2 participants